Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unused front-matter from provider documentation #6238

Merged
merged 7 commits into from
Jul 20, 2022
Merged

Remove unused front-matter from provider documentation #6238

merged 7 commits into from
Jul 20, 2022

Conversation

SarahFrench
Copy link
Contributor

@SarahFrench SarahFrench commented Jul 8, 2022 •
edited
Loading

Description

This PR:

  • removes the layout front-matter from documentation markdown files and template files
  • removes the .erb template files that the layout value refers to
  • removes the sidebar_current front-matter and template files linked to the sidebar navigation in the documentation.
  • removes the .erb template file for the sidebar in the docs

The accepted front-matter values in the Registry documentation are page_title and subcategory. The description field is less clear - I've been told that it should be being used to set meta tags for SEO even though the documentation doesn't mention this, so best to leave them alone!

Other fields like layout and sidebar_current are not supported anymore - e.g. here's an issue in the aws provider that describes sidebar_current needing to be removed. By removing them it'll make it easier to debug documentation issues etc

Impact

Lots of markdown files have the 2 fields deleted from them in this PR

Templates and non-markdown files affected:

This PR should not affect the appearance of documentation in the Registry providing that:

  • All MD files for resources and datasources have subcategory still set
  • All MD files in the guides folder have page_title still set

Open questions & stuff

  • During this work I found this PR updating generation of sidebar_current in tpgtools - I believe that this could be removed, unless I've misunderstood something! Addressed!

  • Also I'm aware this change cannot be tested other than by making a release - sorry about that 😬

  • Depending on how modular-magician works, I think this layout file in the downstream repo might need to be deleted via a PR directly against the downstream?:

References

Checklist

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @shuyama1, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

  • Ensuring it's backwards compatible, covers common error cases, etc.
  • Summarizing the change into a user-facing changelog note.
  • Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

@modular-magician modular-magician requested a review from shuyama1 July 8, 2022 14:59
@shuyama1
Copy link
Member

shuyama1 commented Jul 8, 2022

Looks like the build failed. Are you able to build this locally?

@SarahFrench
Copy link
Contributor Author

Hi @shuyama1 - yes I can build this locally using make terraform, though I've only built the ga version

@SarahFrench
Copy link
Contributor Author

☝️ Latest commit is an extra file @megan07 identified - going to see if deleting that resolves the build issues

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 549 files changed, 4848 deletions(-))
Terraform Beta: Diff ( 549 files changed, 4848 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2094
Passed tests 1865
Skipped tests: 226
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[view]

Tests failed during RECORDING mode:
TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation[view]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]

Please fix these to complete your PR
View the build log or the debug log for each test

@miguelhrocha
Copy link

Hi @shuyama1!

I am Miguel and I work at the Registry team. @SarahFrench asked me to have a look and I can confirm that removing the layout and the sidebar_current front matter makes sense as we currently do not support it in any way or form.

@SarahFrench
Copy link
Contributor Author

SarahFrench commented Jul 18, 2022 •
edited
Loading

Thanks @miguelhrocha !

@shuyama1 - Thanks for bearing with me on this PR!

The remaining thing I don't know is whether this template file need to be removed by a PR directly to the hashicorp/terraform-provider-google repo. It also isn't needed but I can't track its origin from this repo.

Also, there is code from this PR that can be removed as a result of this PR. Should removing this unused code be kept as a separate PR, or should everything related to removing deprecated front matter be in this PR?

@shuyama1
Copy link
Member

Thanks @miguelhrocha !

@shuyama1 - Thanks for bearing with me on this PR!

The remaining thing I don't know is whether this template file need to be removed by a PR directly to the hashicorp/terraform-provider-google repo. It also isn't needed but I can't track its origin from this repo.

I don't think we need a PR against hashicorp/terraform-provider-google repo to remove the template file. It should be automatically removed, as it can be seen in the generated downstream diff result in this PR at #6238 (comment)

Also, there is code from this PR that can be removed as a result of this PR. Should removing this unused code be kept as a separate PR, or should everything related to removing deprecated front matter be in this PR?

Sorry, got a question about this. Looks like the code added in #5890 is introduced to solve hashicorp/terraform-provider-google#10937 where the documentation of some resources are not generated in the correct position in the sidebar. So after merging this PR (removing all unused front-matter and related files), is the appearance of the website sidebar going to remain the same? My understanding is that nothing is going to change on the website and I just want to make sure I understand correctly. Thanks :)

@SarahFrench
Copy link
Contributor Author

Ah yes of course I should have checked the downstream diffs! I overlooked that and was only checking what happened locally on my laptop. Thanks! 😁

As for the expected outcomes of this PR, yes, it should leave the documentation and the sidebar navigation in the Registry unchanged. The stuff I'm removing was used at one point but was deprecated around the time when Terraform provider documentation was migrated from terraform.io to registry.terraform.io (long before my time at HashiCorp).

Here's some explanation about how the frontmatter is used, if it helps give context about my next paragraph

Docs in the Guides subcategory

Documentation in the Guides subcategory is sourced from the website/docs/guides directory (so subcategory frontmatter is unnecessary and can be left unset in those files) and the value of page_title sets what text is shown in the sidebar navigation. E.g. this getting started guide doesn't have subcategory set and page_title matches the first link in this screenshot:

Screenshot 2022-07-19 at 11 15 45

Docs in custom subcategories about resources & data sources

Docs about resources and data sources are all stored in a flat structure in website/docs/r & website/docs/d. They are arranged into subcategories in the Registry sidebar navigation using the value of subcategory in their frontmatter. The value of page_title is not used here - but I think it's fine to leave in those files as the fact they are not used is easier to understand by looking at the Registry's documentation about publishing providers.

E.g. the google_privateca_certificate_template resource has "Certificate Authority Service " as the subcategory value and is shown below in the Registry navigation bar. The page_title value is ignored.

Screenshot 2022-07-19 at 11 46 07

I think my PR will hopefully avoid similar confusion with #5890 , here's the timeline of events showing why it wan't clear at the time that #5890 didn't affect the documentation sidebar as expected:

  1. Jan 20th - the issue you mentioned was created. In v4.7.0 the google_privateca_certificate_template documentation had an incorrect subcategory value "Privateca"
  2. April 4th - the PR Fix sidebar placement of resources generated by tpgtools #5890 that intended to fix the sidebar issue by altering sidebar_current was merged. Totally understandable to expect that to be the fix!
  3. April 13th - this PR Add override PRODUCT_DOCS_SECTION for privateca CertificateTemplate #5937 corrected the value of subcategory (was changed "Privateca" => "Certificate Authority Service")
  4. April 18th - Both of those PRs were included in the v4.18.0 release, where the correct subcategory value was present in the release due to the 2nd PR. This would have changed the appearance in the Registry documentation but it would have been unclear at the time which PR caused the change

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 552 files changed, 4870 deletions(-))
Terraform Beta: Diff ( 552 files changed, 4870 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2109
Passed tests 1879
Skipped tests: 226
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation[view]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]
TestAccFirebaserulesRelease_BasicRelease[view]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[view]

Please fix these to complete your PR
View the build log or the debug log for each test

@shuyama1
Copy link
Member

@SarahFrench Thank you so much for the detailed explanation! It's super clear now. Then I think this PR is good to go. Thanks for making the changes!

@shuyama1
Copy link
Member

Thanks @miguelhrocha for taking a look!

@shuyama1 shuyama1 merged commit c754c98 into GoogleCloudPlatform:main Jul 20, 2022
This was referenced Jul 20, 2022
Merged
Merged
Merged
geojaz pushed a commit to geojaz/magic-modules that referenced this pull request Jul 21, 2022
@SarahFrench @geojaz
Remove unused front-matter from provider documentation (GoogleCloudPl…
16172a4 
…atform#6238)
@SarahFrench SarahFrench mentioned this pull request Jan 20, 2023
Merged
4 tasks
@SarahFrench SarahFrench deleted the remove-layout-frontmatter branch March 26, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shuyama1 shuyama1 shuyama1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SarahFrench @modular-magician @shuyama1 @miguelhrocha